Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #1994 #4874

Closed
wants to merge 4 commits into from
Closed

Fixes #1994 #4874

wants to merge 4 commits into from

Conversation

flyx
Copy link
Contributor

@flyx flyx commented Oct 9, 2016

This PR adds gorgeEx and staticReadEx which both return a tuple of output and result code. Furthermore, it add an optional parameter onErrorBreak to gorge and staticRead, because that was cheap to add when implementing gorgeEx functionality.

staticReadEx and gorgeEx will still break if executing the command raises an OSError.

This PR includes #4872.

@yglukhov
Copy link
Member

So in what way #4871 is fixed? Any chance to note that in news.txt?

@flyx
Copy link
Contributor Author

flyx commented Oct 25, 2016

@yglukhov sorry, just saw that comment. The fix for #4871 has already been merged by #4872, so this seems to be the wrong place to discuss it. @Araq already documented it in the news.

@Araq
Copy link
Member

Araq commented Oct 25, 2016

I think this implementation is convoluted. Internally opcGorge can be changed to opcGorgeEx and a wrapper gorge can call gorgeEx. I'm also not too happy with this Ex naming convention.

@flyx
Copy link
Contributor Author

flyx commented Oct 25, 2016

@Araq: I had problems determining which proc was called without splitting opcGorge and opcGorgeEx. Not sure what and how you want it to be changed.

What would you prefer as name for the extended procs?

 * moved common opcGorge[Ex] code to template
 * renamed opGorge to opGorgeEx
 * splitted opcGorge / opcGorgeEx implementations
@Araq
Copy link
Member

Araq commented Jan 7, 2017

Implemented it differently. Read and learn.

@Araq Araq closed this Jan 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants